-
Notifications
You must be signed in to change notification settings - Fork 922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set language for tabs in URL query #1202
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
e093e00
to
83b6e64
Compare
1a01900
to
8971ddd
Compare
This is not working for me. It seems the persistent tab functionality is broken, but has nothing to do with your PR. I'm seeing an error on console: I'm going to try and figure out what's going on. The page you linked to also shows an error, but a different one: That one is likely caused by a recent update to the tabpane shortcode that changed |
Looks like we're not actually using the latest version of docsy in our code right now either. Trying to update all the things right now. Let me put this in draft, and I'll ping you when I can verify it works. Thanks. |
25807c5
to
0d31df6
Compare
Ok, I got the selenium.dev example working. We're using a version of docsy from a couple months ago, but the only commits that might change things are in #1037, and I don't see anything specific there. My previous PR commit didn't set the local session properly, but that shouldn't result in the error you are seeing... I'm not a JS dev, though and don't know what Also as a side note, is there a reason for the change from |
0d31df6
to
765c557
Compare
This doesn't work for me either (with tip of |
765c557
to
970e565
Compare
I tried this on you website and it works as expected, but for some reason it doesn't work when I try it with the docsy user guide. It's probably related to another issue and not your implementation. I gonna take another look today and get it merged. |
Well, We're not using the latest version of Docsy, so it's possible something else is getting in the way. :( I don't have time to dig into it, so we can put this into draft if you want and I can revisit it once @diemol updates us to the latest version of Docsy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this on you website and it works as expected, but for some reason it doesn't work when I try it with the docsy user guide.
I can't confirm- For me, setting the language via URL parameter works as expected, both with a module site from scratch and with the user guide.
Overall, this PR looks good to me and is a useful addition to the tabpane
shortcode.
static/js/tabpane-persist.js
Outdated
}); | ||
if (params.language !== null) { | ||
activeLanguage = params.language; | ||
localStorage.setItem('active_language', activeLanguage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uncertain if we should persist the active language given when given as URL parameter.
Currently, the language is always persisted, even if the user set persistLang=false
for the tab pane. So maybe it is better to not persist the language at all when given as URL parameter.
Must be something with my local environment, the Netlify preview also works fine.
I think it's OK to persist the language. I feel that having it in the URL explicitly makes a language selection that supersedes the |
Maybe we can unclear it if they switch to another tab and persistLang isn't set? persistLang just makes much more sense when used in conjunction with #1227. |
970e565
to
7c520fe
Compare
7c520fe
to
254bae2
Compare
I've been using this in custom-hacked code for a while, but now that I'm upgrading to the most recent tabpane implementation (nice work on that!), I figured I should submit it to see if I can avoid adding my own hacks. :)
This would allow users to link directly to the language they want:https://selenium.dev/documentation/webdriver/actions_api/mouse/?language=ruby#forward-clickUpdate:
New syntax uses
tab
. (seems easier than the more descriptiveactiveTab
):https://selenium.dev/documentation/webdriver/actions_api/mouse/?tab=ruby#forward-click